console: prevent internal timers storage from growing by narqo · Pull Request #3562 · nodejs/node

narqo · 2015-10-28T13:01:53Z

The instance of Console class doesn't remove links to arrays that come from hrtimer after timeEnd had been called.

As far as I understand, such links won't be collected by v8's gc.

cjihrig · 2015-10-28T13:25:40Z

Changes LGTM, although a regression test would be nice. This is a behavior change (semver-major) that gets us in line with what Chrome does.

evanlucas · 2015-10-28T14:39:27Z

Agree with @cjihrig. A test is necessary. Otherwise LGTM though

narqo · 2015-10-28T15:47:29Z

@cjihrig @evanlucas the only test I can imagine is one that checks internal _times map length (size) on timeEnd call (or checks the particular key was removed from the map). Does such checks of module's internal behaviour are suitable?

evanlucas · 2015-10-28T15:48:45Z

yea I would say just call console.time a few times and then verify the size of this._times and then call console.timeEnd a few times and do another assert

narqo · 2015-10-28T16:43:35Z

I have added a test which verifies that timeEnd() calls doesn't leave any new links in the map.

cjihrig · 2015-10-28T17:06:46Z

Should probably use strictEqual() in the test. Other than that, LGTM if the CI is happy.

cjihrig · 2015-10-28T17:07:51Z

test/parallel/test-console.js

Actually, please make this const too.

narqo · 2015-10-28T19:40:25Z

@cjihrig yep!

bnoordhuis · 2015-10-28T20:17:36Z

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/642/

bnoordhuis · 2015-10-28T20:18:51Z

@cjihrig @evanlucas Can you explain why this is semver-major?

cjihrig · 2015-10-28T20:20:38Z

Currently, you can reuse a timer, for example after each iteration of a loop. With this change you would have to create a new timer each time.

narqo · 2015-10-28T20:37:23Z

@cjihrig do you mean the cases like this:

console.time('label');
console.timeEnd('label');
console.timeEnd('label');
console.timeEnd('label');

cjihrig · 2015-10-28T20:43:23Z

Yes. I'd be willing to bet there are people out there relying on that behavior.

narqo · 2015-10-28T20:45:13Z

To be honest I've never thought about such usecase. At least Blink's implementation of console.time() doesn't work this way: undefined is returned after first timeEnd() call.

Fishrock123 · 2015-10-28T20:50:29Z

To be honest I've never thought about such usecase. At least Blink's implementation of console.time() doesn't work this way: undefined is returned after first timeEnd() call.

Note: we don't follow the spec anyways. See: #3473

cjihrig · 2015-11-09T18:24:48Z

@Fishrock123 did we decide to move forward with this change?

ChALkeR · 2015-11-09T18:34:31Z

The spec says that the timer should be stopped.
Both Chrome and Firefox stop the timer, and don't output anything on subsequent console.timeEnd calls.
The current implementation actually leaks memory if one creates and stops a significant amount of timers.

+1 for this change, a semver-major though.

cjihrig · 2015-11-13T16:00:44Z

@narqo could you rebase this please.

The instance of `Console` class doesn't remove links to arrays that come from hrtimer after `timeEnd` had been called.

narqo · 2015-11-13T16:33:13Z

🆙

targos · 2015-11-13T16:40:18Z

LGTM

We may also want to stop throwing on console.timeEnd calls with inexistant labels but that can be done in another PR.

Currently, console timers that have been ended with timeEnd() are not removed. This has the potential to leak memory. This commit deletes ended timers from the containing Map. PR-URL: #3562 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

cjihrig · 2015-11-13T16:44:06Z

Thanks! Landed in a5cce79

Unintended functionality was removed from console.endTime by nodejs#3562. Prior to that, you could call console.endTime multiple times for the same label.

Unintended functionality was removed from console.endTime by #3562. Prior to that, you could call console.endTime multiple times for the same label. PR-URL: #6454 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>

Unintended functionality was removed from console.endTime by nodejs#3562. Prior to that, you could call console.endTime multiple times for the same label. PR-URL: nodejs#6454 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Lindstaedt <robert.lindstaedt@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Jeremy Whitlock <jwhitlock@apache.org>

mscdex added the console Issues and PRs related to the console subsystem. label Oct 28, 2015

targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 28, 2015

narqo force-pushed the console-drain-timers branch from 69162e1 to d4a62d7 Compare October 28, 2015 16:39

cjihrig reviewed Oct 28, 2015
View reviewed changes

test/parallel/test-console.js Outdated

Copy link

Contributor

cjihrig Oct 28, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, please make this const too.

narqo force-pushed the console-drain-timers branch from d4a62d7 to 3a171f9 Compare October 28, 2015 19:39

console: prevent internal timers storage from growing

474c066

The instance of `Console` class doesn't remove links to arrays that come from hrtimer after `timeEnd` had been called.

narqo force-pushed the console-drain-timers branch from 3a171f9 to 474c066 Compare November 13, 2015 16:33

cjihrig closed this Nov 13, 2015

Fishrock123 mentioned this pull request Nov 26, 2015

deps: upgrade npm to 3.5.0 #4032

Closed

Fishrock123 mentioned this pull request Dec 15, 2015

v5.3.0 Release Proposal (Stable) #4281

Closed

jasnell mentioned this pull request Mar 17, 2016

Planning for v6 #5766

Closed

ben-page mentioned this pull request Apr 28, 2016

undocumented console.timeEnd() change in 6.x #6452

Closed

ben-page mentioned this pull request Apr 28, 2016

doc: added note warning about change to console.endTime() #6454

Closed

2 tasks

targos mentioned this pull request Jan 28, 2017

Intermediate values with console.time whatwg/console#84

Closed

snyk-bot mentioned this pull request Jan 21, 2023

[Snyk] Upgrade d3 from 5.16.0 to 7.8.0 Supermajestcd/node#7

Open

aliscco mentioned this pull request Apr 25, 2023

[Snyk] Fix for 14 vulnerabilities aliscco/alisco-node#174

Open

leonardoadame mentioned this pull request Jul 7, 2023

[Snyk] Fix for 14 vulnerabilities leonardoadame/node#58

Open

Uh oh!

Conversation

narqo commented Oct 28, 2015

Uh oh!

cjihrig commented Oct 28, 2015

Uh oh!

evanlucas commented Oct 28, 2015

Uh oh!

narqo commented Oct 28, 2015

Uh oh!

evanlucas commented Oct 28, 2015

Uh oh!

narqo commented Oct 28, 2015

Uh oh!

cjihrig commented Oct 28, 2015

Uh oh!

cjihrig Oct 28, 2015

Choose a reason for hiding this comment

Uh oh!

narqo commented Oct 28, 2015

Uh oh!

bnoordhuis commented Oct 28, 2015

Uh oh!

bnoordhuis commented Oct 28, 2015

Uh oh!

cjihrig commented Oct 28, 2015

Uh oh!

narqo commented Oct 28, 2015

Uh oh!

cjihrig commented Oct 28, 2015

Uh oh!

narqo commented Oct 28, 2015

Uh oh!

Fishrock123 commented Oct 28, 2015

Uh oh!

cjihrig commented Nov 9, 2015

Uh oh!

ChALkeR commented Nov 9, 2015

Uh oh!

cjihrig commented Nov 13, 2015

Uh oh!

narqo commented Nov 13, 2015

Uh oh!

targos commented Nov 13, 2015

Uh oh!

cjihrig commented Nov 13, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants